Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SubscribePage.data and accessors; repository adds findPageWithData and cursor pagination; new SubscribePageConfigMigrationService migrates config↔page-data; SubscribePageManager gains findPage and syncPageData and drops Translator; ConfigProvider accepts string namespaced keys; services/parameters wiring and expanded unit tests added. ChangesSubscription page model, repository, manager, migration, config, and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Domain/Subscription/Repository/SubscriberPageRepository.php (1)
41-49: 💤 Low valueHeads-up on
totalsemantics +lastIdparameter usage.Two things worth a glance:
$countQb = clone $queryBuilder;is cloned before theandWhere('p.id > :afterId')is attached, so$totalends up being the count of all pages — not the count after the cursor. That's actually fine for cursor pagination if you want the consumer to know the total set size, but it's easy to misread; a short comment would help.$filter->getLastId()is called repeatedly — minor, but caching it in a local would also avoid drift if the filter object ever mutates between calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php` around lines 41 - 49, In getFilteredAfterId: cache $filter->getLastId() into a local (e.g. $afterId) and use that variable for all subsequent calls to avoid repeated method calls and potential mutation drift; also add a short comment next to the $countQb = clone $queryBuilder (or adjust cloning placement) clarifying whether $total is intended to be the overall count or the post-cursor count—if you want total after the cursor, move the clone to after applying andWhere('p.id > :afterId'), otherwise keep current clone and document the semantics.src/Domain/Subscription/Service/Manager/SubscribePageManager.php (1)
40-66: 💤 Low valueMixing
pageDataRepository->persist(...)withentityManager->remove(...)reads oddly.
syncPageDataschedules creates through the repository ($this->pageDataRepository->persist($pageData)) but schedules deletes directly against$this->entityManager->remove($pageData). Both are fine from a domain-purity standpoint (no flush, no DDL — that's allowed by the guidelines), but it's a little jarring to read. IfSubscriberPageDataRepositoryalready has (or could grow) aremove()method, going through the repo for both keeps the abstraction consistent and theentityManagerdependency a bit less load-bearing.#!/bin/bash ast-grep --pattern $'class SubscriberPageDataRepository { $$$ }'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php` around lines 40 - 66, Replace direct use of $this->entityManager->remove(...) in syncPageData with a repository-level deletion to keep persistence abstraction consistent: add a remove(SubscribePageData $pageData): void method to SubscriberPageDataRepository (or ensure it exists) and call $this->pageDataRepository->remove($pageData) inside SubscribePageManager::syncPageData instead of $this->entityManager->remove($pageData); leave persist calls as-is and do not change flushing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php`:
- Around line 92-106: The loadPageData method is dropping the first row's data
because array_shift($result)['page'] removes that row; update loadPageData
(method loadPageData, class SubscriberPageRepository, working with SubscribePage
and setData) to obtain the SubscribePage without mutating the $result array
(e.g. read $result[0]['page'] or otherwise extract 'page' from the first
element) and then iterate over the full $result to collect every 'data' entry
before calling $page->setData($data) so the first row's data is preserved.
- Around line 66-82: The grouping in getFilteredAfterId is creating
duplicate/invalid buckets by indexing both $page->getId() and $data->getId();
change the grouping so each bucket is keyed only by the page id (use
$page->getId()) and aggregate associated data entries into that same bucket
(append ['data'=> $data] to the page's array) while skipping data-only entries
when no page is present, so loadPageData always receives a group starting with a
'page' element; update getFilteredAfterId's loop to only create/append to
$grouped[$page->getId()] and stop creating $grouped[$data->getId()], ensuring
compatibility with SubscribePageData/SubscribePageManager::setPageData and
syncPageData expectations.
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 55-60: The call to SubscribePageData::setId uses $page->getId()
which is ?int and can be null for a new page; update syncPageData to mirror the
existing setPageData behavior by passing (int)$page->getId() into
SubscribePageData::setId (or add an explicit guard/assert that the page is
persisted before calling syncPageData) so setId never receives null and avoids a
TypeError.
In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php`:
- Around line 347-350: The test uses the deprecated withConsecutive on the
entity manager mock; replace that expectation by capturing the actual arguments
passed to EntityManagerInterface::remove via a willReturnCallback and then
assert the captured calls equal [$pageData1, $pageData2]. Concretely, in
SubscribePageManagerTest replace the ->withConsecutive([...]) on the
$this->entityManager->expects(...)->method('remove') mock with
->willReturnCallback(function($arg) use (&$calls) { $calls[] = $arg; }) and
after exercising the SUT assert that $calls contains $pageData1 then $pageData2.
---
Nitpick comments:
In `@src/Domain/Subscription/Repository/SubscriberPageRepository.php`:
- Around line 41-49: In getFilteredAfterId: cache $filter->getLastId() into a
local (e.g. $afterId) and use that variable for all subsequent calls to avoid
repeated method calls and potential mutation drift; also add a short comment
next to the $countQb = clone $queryBuilder (or adjust cloning placement)
clarifying whether $total is intended to be the overall count or the post-cursor
count—if you want total after the cursor, move the clone to after applying
andWhere('p.id > :afterId'), otherwise keep current clone and document the
semantics.
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 40-66: Replace direct use of $this->entityManager->remove(...) in
syncPageData with a repository-level deletion to keep persistence abstraction
consistent: add a remove(SubscribePageData $pageData): void method to
SubscriberPageDataRepository (or ensure it exists) and call
$this->pageDataRepository->remove($pageData) inside
SubscribePageManager::syncPageData instead of
$this->entityManager->remove($pageData); leave persist calls as-is and do not
change flushing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7ca3a94-7f3e-4081-b51b-c76c40753239
📒 Files selected for processing (4)
src/Domain/Subscription/Model/SubscribePage.phpsrc/Domain/Subscription/Repository/SubscriberPageRepository.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Subscription/Service/Manager/SubscribePageManager.php (1)
55-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
SubscribePage::$datain sync with the diff you just applied.
syncPageData()updates persistence state, but it never rebuilds the new in-memorySubscribePage::getData()container. After a create/delete path, callers can keep using$pageand still see stale entries until they reload it.💡 Minimal fix sketch
public function syncPageData(array $data, SubscribePage $page): void { $existingPageData = []; + $syncedPageData = []; foreach ($this->getPageData($page) as $pageData) { $existingPageData[$pageData->getName()] = $pageData; } foreach ($data as $pageDataKey => $value) { if (isset($existingPageData[$pageDataKey])) { $pageData = $existingPageData[$pageDataKey]; $pageData->setData($value); unset($existingPageData[$pageDataKey]); + $syncedPageData[] = $pageData; continue; } $pageData = (new SubscribePageData()) ->setId($page->getId()) ->setName($pageDataKey) ->setData($value); $this->pageDataRepository->persist($pageData); + $syncedPageData[] = $pageData; } foreach ($existingPageData as $pageData) { $this->entityManager->remove($pageData); } + + $page->setData($syncedPageData); if ($this->subscribePageConfigMigrationEnabled) { $this->configMigrationService->copyToConfig(page: $page, data: $data); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php` around lines 55 - 84, syncPageData updates DB entries but never updates the in-memory SubscribePage::$data, so callers see stale data; after applying creates/updates/deletes in syncPageData (the method in SubscribePageManager) update the SubscribePage instance's data container to reflect the new state — e.g. rebuild the data array from the final $data input (or from the adjusted $existingPageData plus newly created entries) and call $page->setData(...) so SubscribePage::getData() returns the current values; ensure this happens just before calling configMigrationService->copyToConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php`:
- Around line 102-103: In ConfigProvider::getValue path where you call
getValue($parentKey), guard against boolean-style parent keys by wrapping the
call in a try/catch that specifically catches InvalidArgumentException and
returns null (or delegates to isEnabled handling) instead of letting the
exception bubble; locate the getValue($parentKey) invocation in ConfigProvider
and add the try/catch around it so boolean-like inputs such as
"maintenancemode:1" do not cause a runtime exception.
In `@src/Domain/Subscription/Service/SubscribePageConfigMigrationService.php`:
- Around line 82-84: The Domain service SubscribePageConfigMigrationService
currently calls $this->entityManager->flush() inside its migration method;
remove that flush so the domain service remains side-effect free
(prepare/persist/remove only) and instead return or expose the $hasChanges /
scheduled-operations result (e.g., boolean or array of changed entities) so the
Application/handler layer can call flush(); update the method in
SubscribePageConfigMigrationService to stop finalizing the DB and adjust tests
to assert the returned change state and that persistence was scheduled rather
than expecting flush() to have been invoked.
---
Outside diff comments:
In `@src/Domain/Subscription/Service/Manager/SubscribePageManager.php`:
- Around line 55-84: syncPageData updates DB entries but never updates the
in-memory SubscribePage::$data, so callers see stale data; after applying
creates/updates/deletes in syncPageData (the method in SubscribePageManager)
update the SubscribePage instance's data container to reflect the new state —
e.g. rebuild the data array from the final $data input (or from the adjusted
$existingPageData plus newly created entries) and call $page->setData(...) so
SubscribePage::getData() returns the current values; ensure this happens just
before calling configMigrationService->copyToConfig.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7dde6c6f-30e3-4851-9b36-e4c6f478cac1
📒 Files selected for processing (8)
config/parameters.yml.distconfig/services/services.ymlsrc/Domain/Configuration/Service/Provider/ConfigProvider.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phpsrc/Domain/Subscription/Service/SubscribePageConfigMigrationService.phptests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.phptests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Domain/Configuration/Service/Provider/ConfigProvider.php (1)
94-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle boolean parent keys in namespace fallback to prevent runtime exceptions.
At Line 102,
getValue($parentKey)can still throwInvalidArgumentExceptionfor boolean parent keys (for example,maintenancemode:1) and currently bubbles out.Proposed fix
if (str_contains($keyValue, ':')) { [$parent] = explode(':', $keyValue, 2); try { $parentKey = ConfigOption::from($parent); } catch (ValueError) { return null; } - return $this->getValue($parentKey); + try { + return $this->getValue($parentKey); + } catch (InvalidArgumentException) { + return null; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php` around lines 94 - 103, The namespace fallback branch in ConfigProvider tries to convert the parent token via ConfigOption::from($parent) and then calls $this->getValue($parentKey), but getValue can throw InvalidArgumentException for boolean-like parent keys (e.g., "maintenancemode:1") and currently bubbles up; wrap the $this->getValue($parentKey) call in a try/catch that catches InvalidArgumentException and returns null on failure so the method gracefully falls back, keeping the existing catch for ValueError around ConfigOption::from intact (refer to ConfigOption::from and ConfigProvider::getValue to locate the code).
🧹 Nitpick comments (1)
tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php (1)
71-76: 💤 Low value
withConsecutive()is deprecated in PHPUnit 9.6 and removed in 10.This new test uses the same deprecated pattern flagged elsewhere. Consider using a callback-based approach to future-proof the test.
Suggested refactor
+ $callCount = 0; $this->pageRepository ->expects($this->exactly(2)) ->method('findPageWithData') - ->withConsecutive([123], [123]) - ->willReturnOnConsecutiveCalls($page, $refetchedPage); + ->with(123) + ->willReturnCallback(function () use (&$callCount, $page, $refetchedPage) { + return ++$callCount === 1 ? $page : $refetchedPage; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php` around lines 71 - 76, The test currently uses the deprecated withConsecutive() on the mock for pageRepository->findPageWithData; replace this with a callback-based stub: remove withConsecutive() and use willReturnCallback on the mock for findPageWithData, implement a small closure that captures a local counter (e.g., $call = 0) and increments on each invocation, inspects the incoming argument(s) to ensure it's the expected page id (123) and returns $page on the first call and $refetchedPage on the second; keep the ->expects($this->exactly(2)) expectation and ensure the closure signature matches findPageWithData so the test behavior is identical without using withConsecutive().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/Domain/Configuration/Service/Provider/ConfigProvider.php`:
- Around line 94-103: The namespace fallback branch in ConfigProvider tries to
convert the parent token via ConfigOption::from($parent) and then calls
$this->getValue($parentKey), but getValue can throw InvalidArgumentException for
boolean-like parent keys (e.g., "maintenancemode:1") and currently bubbles up;
wrap the $this->getValue($parentKey) call in a try/catch that catches
InvalidArgumentException and returns null on failure so the method gracefully
falls back, keeping the existing catch for ValueError around ConfigOption::from
intact (refer to ConfigOption::from and ConfigProvider::getValue to locate the
code).
---
Nitpick comments:
In `@tests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.php`:
- Around line 71-76: The test currently uses the deprecated withConsecutive() on
the mock for pageRepository->findPageWithData; replace this with a
callback-based stub: remove withConsecutive() and use willReturnCallback on the
mock for findPageWithData, implement a small closure that captures a local
counter (e.g., $call = 0) and increments on each invocation, inspects the
incoming argument(s) to ensure it's the expected page id (123) and returns $page
on the first call and $refetchedPage on the second; keep the
->expects($this->exactly(2)) expectation and ensure the closure signature
matches findPageWithData so the test behavior is identical without using
withConsecutive().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85869e0d-e1b8-4472-8c17-e3523cddc4d3
📒 Files selected for processing (8)
config/parameters.yml.distconfig/services/services.ymlsrc/Domain/Configuration/Service/Provider/ConfigProvider.phpsrc/Domain/Subscription/Service/Manager/SubscribePageManager.phpsrc/Domain/Subscription/Service/SubscribePageConfigMigrationService.phptests/Unit/Domain/Configuration/Service/Provider/ConfigProviderTest.phptests/Unit/Domain/Subscription/Service/Manager/SubscribePageManagerTest.phptests/Unit/Domain/Subscription/Service/SubscribePageConfigMigrationServiceTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests
Thanks for contributing to phpList!